Skip to content

feat: add database migration support and connection utilities#17

Merged
ankaisen merged 11 commits intomainfrom
feature/1204-database-migration
Feb 4, 2026
Merged

feat: add database migration support and connection utilities#17
ankaisen merged 11 commits intomainfrom
feature/1204-database-migration

Conversation

@SparkZou
Copy link
Copy Markdown
Contributor

@SparkZou SparkZou commented Jan 25, 2026

Summary

Add database configuration and Alembic migration setup for the memU server.

Changes

Database Configuration (app/database.py)

  • Add get_database_url() function to construct database URL from environment variables
  • Support both DATABASE_URL direct config and individual variables
  • URL-encode credentials using urllib.parse.quote(..., safe="") for proper userinfo encoding
  • Normalize PostgreSQL URL schemes (postgres://, postgresql://, postgresql+asyncpg://) to postgresql+psycopg://
  • Configure async SQLAlchemy engine with connection pooling

Model Base Class (app/models/base.py)

  • Create side-effect-free Base class for SQLAlchemy models
  • Enables safe import in alembic/env.py without triggering DB connection

Alembic Migration Setup

  • Add alembic.ini configuration file
  • Add alembic/env.py with environment variable support
  • Configure target_metadata = Base.metadata for autogenerate support
  • Auto-convert async/legacy URLs to sync postgresql+psycopg:// for migrations
  • Add alembic/versions/.gitkeep to preserve directory in git

Environment Variables

Variable Default Description
DATABASE_URL - Full database connection URL (takes priority)
DATABASE_HOST - Database hostname
DATABASE_PORT 5432 Database port
DATABASE_USER - Database username
DATABASE_PASSWORD - Database password
DATABASE_NAME - Database name
STORAGE_PATH ./data Storage directory (also supports legacy MEMU_STORAGE_DIR)

Known Limitations

The following are intentionally deferred to a future refactoring PR:

  1. Lazy initialization - DATABASE_URL and engine are created at import time. This is marked with TODO comments for future optimization.

  2. OPENAI_API_KEY validation at import time - Validation runs at module import. Moving to FastAPI lifespan is planned.

Testing

  • make check passes (lint, type check, dependency check)
  • make test passes

Add Alembic for database schema migrations and centralize database connection logic.

## Changes

### Database Migration (Alembic)
- Add alembic.ini configuration file
  * Script location: alembic/
  * File template: %%%(rev)s_%%%(slug)s
  * Timezone support: UTC

- Add alembic/env.py migration environment
  * Async SQLAlchemy support
  * TODO: Connect target_metadata to models for autogenerate

- Add alembic/script.py.mako migration template
  * Standard Alembic revision template

### Database Connection Module (app/database.py)
- Add get_database_url() utility function
  * Centralized URL construction logic (DRY principle)
  * URL-encodes credentials to handle special characters
  * Supports DATABASE_URL env var or individual components
  * Security: Prevents credential leak in error messages

### Application Updates (app/main.py)
- Refactor to use shared get_database_url() function
- Remove duplicate database URL construction code
- Improve error message clarity without exposing credentials

## Security Improvements
- URL-encode database credentials (urllib.parse.quote_plus)
- Prevent password exposure in error messages
- Secure connection string handling

## Benefits
- Consistent database URL handling across app
- Support for passwords with special characters
- Easy to test and maintain (single source of truth)
- Ready for schema version control

## Usage
```bash
# Create a new migration
alembic revision -m "description"

# Apply migrations
alembic upgrade head

# Rollback migrations
alembic downgrade -1
```

## TODO
- Implement lazy database initialization
- Add environment variable support in alembic.ini
- Connect target_metadata for autogenerate feature

## Files Changed (5 files)
- alembic.ini: Alembic configuration
- alembic/env.py: Migration environment setup
- alembic/script.py.mako: Migration template
- app/database.py: NEW - Database connection utilities
- app/main.py: Refactored to use shared database module
Copilot AI review requested due to automatic review settings January 25, 2026 05:45
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds initial database migration scaffolding (Alembic) and centralizes database connection URL construction to support consistent DB configuration across the app.

Changes:

  • Introduces Alembic configuration + environment + revision template files.
  • Adds app/database.py with get_database_url() and SQLAlchemy async engine/session setup.
  • Refactors app/main.py to use the shared DB URL utility and updated service initialization.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
app/main.py Uses centralized DB URL utility and updates app/service initialization; adjusts storage env var.
app/database.py New DB utilities + SQLAlchemy async engine/session factory.
alembic.ini Adds Alembic configuration scaffold (currently missing a usable URL configuration).
alembic/env.py Adds Alembic environment script (currently sync engine setup and expects sqlalchemy.url).
alembic/script.py.mako Adds Alembic revision file template.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/database.py Outdated
Comment thread alembic.ini
Comment thread alembic.ini
Comment thread alembic/env.py Outdated
Comment thread alembic/env.py Outdated
Comment thread app/database.py Outdated
Comment thread app/database.py
Comment thread alembic.ini
Comment thread app/main.py Outdated
- Fix DATABASE_PORT default from 54320 to standard 5432
- Update alembic/env.py to read DATABASE_URL from environment variables
- Use sync engine in alembic for migrations (not async)
- Add type assertions for mypy compatibility
- Ignore DEP003 for app package in deptry config
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread alembic/env.py Outdated
Comment thread app/database.py Outdated
Comment thread app/database.py Outdated
Comment thread app/database.py
Comment thread alembic/env.py
Comment thread alembic/env.py Outdated
Comment thread alembic.ini
Comment thread app/database.py
Comment thread app/main.py
- Remove unused logger import in app/database.py
- Add URL encoding for credentials in alembic/env.py
- Convert async DB URL to sync for Alembic migrations
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread alembic/env.py Outdated
Comment thread alembic/env.py Outdated
Comment thread alembic.ini Outdated
Comment thread alembic.ini
Comment thread app/database.py
Comment thread app/database.py
Comment thread app/main.py
Comment thread app/main.py
- Normalize PostgreSQL URL schemes in app/database.py and alembic/env.py
  - Handle postgres://, postgresql://, postgresql+asyncpg:// formats
  - Convert all to postgresql+psycopg:// for consistency
- Add URL encoding for credentials in alembic/env.py
- Update alembic.ini comments to reflect actual implementation
- Add backward compatibility for MEMU_STORAGE_DIR env var in app/main.py
Copilot AI review requested due to automatic review settings January 31, 2026 22:57
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/database.py
Comment thread app/main.py
Comment thread alembic/env.py Outdated
Comment thread alembic/env.py Outdated
Comment thread app/database.py
- Use quote(..., safe='') instead of quote_plus() for URL userinfo encoding
  (quote_plus encodes spaces as '+' which is incorrect for URL userinfo)
- Add asyncpg to psycopg conversion in app/database.py
  (consistent with alembic/env.py, asyncpg is not a project dependency)
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread alembic/env.py Outdated
Comment thread app/database.py
…erate

- Create app/models/base.py with Base class (no DB connection on import)
- Update app/database.py to import Base from app.models.base
- Update alembic/env.py to set target_metadata = Base.metadata
- This enables 'alembic revision --autogenerate' to detect model changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/database.py Outdated
Comment thread app/main.py
Comment thread app/database.py
SQLAlchemy 2.x async_sessionmaker does not support the autocommit argument.
Use explicit transaction scopes instead.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread alembic.ini Outdated
Comment thread app/models/base.py Outdated
Comment thread app/database.py
Comment thread app/database.py Outdated
Comment thread alembic/env.py Outdated
Replace 'in' operator with startswith() to avoid false positives when
checking for asyncpg driver in database URLs. This ensures only URLs
that actually start with postgresql+asyncpg:// are converted.
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread alembic/env.py Outdated
Comment on lines +49 to +53
db_host = os.getenv("DATABASE_HOST", "localhost")
db_port = os.getenv("DATABASE_PORT", "5432")
db_user = os.getenv("DATABASE_USER", "postgres")
db_pass = os.getenv("DATABASE_PASSWORD", "postgres")
db_name = os.getenv("DATABASE_NAME", "memu")
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In get_sync_database_url(), the individual variable fallback uses hard-coded defaults (localhost/postgres/postgres/memu). This contradicts the PR description’s env var table (no defaults for host/user/password/name) and can cause Alembic to run against an unintended database when env vars are missing. Consider matching app.database.get_database_url() behavior (raise with a clear error) or document these defaults explicitly and ensure they’re safe.

Copilot uses AI. Check for mistakes.
Comment thread alembic/env.py
Comment thread app/main.py
Comment thread app/database.py
Remove hard-coded defaults from get_sync_database_url() and add explicit
validation for required environment variables. This ensures consistent
behavior between Alembic migrations and the application, preventing
accidental connections to unintended databases.
@SparkZou SparkZou requested a review from ankaisen February 1, 2026 07:58
Copy link
Copy Markdown
Contributor

@ankaisen ankaisen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SparkZou Thank you! LGTM 👍

@ankaisen ankaisen merged commit 258439c into main Feb 4, 2026
1 check passed
@ankaisen ankaisen deleted the feature/1204-database-migration branch February 4, 2026 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants